Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more ways to read scheduled enclave #2620

Merged
merged 18 commits into from
Apr 11, 2024

Conversation

Traf333
Copy link
Contributor

@Traf333 Traf333 commented Mar 26, 2024

Context

this PR adds 2 ways a way to read scheduled enclave list

  1. via binary:./bitacross-worker scheduled-enclave
  2. via RPC: echo '{"jsonrpc":"2.0","method":"state_getScheduledEnclaveList","id":1}' | websocat -n1 -k -B 99999999 wss://localhost:2000

Copy link

linear bot commented Mar 26, 2024

@kziemianek
Copy link
Member

Do you plan adding this method to tee-worker?

@Traf333
Copy link
Contributor Author

Traf333 commented Apr 8, 2024

Do you plan adding this method to tee-worker?
@kziemianek I'd say yes and have only one concern, how would it work with multi-worker setup. Does scheduled_enclave_sealed.bin synchronises for each worker once it's run?

@silva-fj
Copy link
Contributor

silva-fj commented Apr 8, 2024

Do you plan adding this method to tee-worker?
@kziemianek I'd say yes and have only one concern, how would it work with multi-worker setup. Does scheduled_enclave_sealed.bin synchronises for each worker once it's run?

It will once we merge #2628

@Traf333
Copy link
Contributor Author

Traf333 commented Apr 9, 2024

at the end decided to keep only one method to obtain scheduled enclave list as through the binary it seems unstable

@@ -177,6 +176,13 @@ where
}))
});

io.add_sync_method("state_getScheduledEnclaveList", move |_: Params| {
debug!("worker_api_direct rpc was called: state_getScheduledEnclaveList");
let value = format!("{:?}", &GLOBAL_SCHEDULED_ENCLAVE.registry.read());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This results in something like this:
Ok({0: [177, 155, 22, 22, 112, 239, 100, 252, 239, 19, 10, 78, 170, 5, 181, 229, 13, 234, 219, 91, 29, 160, 23, 147, 133, 219, 42, 14, 151, 253, 19, 134]})

  1. You should return only value contained in Result not Result itself.
  2. In my opinion value should be serialized directly, not formated to string then serialized.
  3. Can we have an integration test for this ?

@Traf333
Copy link
Contributor Author

Traf333 commented Apr 10, 2024

Included integration tests which also addresses next linear issue P-645 : Add test case for scheduled-enclave setting where tests covered:

  • state_getScheduledEnclave
  • couple of invalid and valid scenarios for state_setScheduledEnclave

Copy link
Member

@kziemianek kziemianek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done

import { createJsonRpcRequest, nextRequestId } from './common/helpers';

describe('Scheduled Enclave', function () {
let context: IntegrationTestContext = undefined as any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we define the type and then cast it to any?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question 🤔 this one was copy-paste from another test. I just created chore task to cleanup our tests, as I've also seen some fixme's were copied from one test to another

@Traf333 Traf333 merged commit 8bb43b0 into dev Apr 11, 2024
28 of 30 checks passed
@Traf333 Traf333 deleted the p-644-add-more-ways-to-read-scheduled-enclave branch April 11, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants